https://searchfox.org/mozilla-central/search?q=NS_IMETHOD%28_%5C%28%5B%5E%29%5D%2B%5C%29%29%3F+%5B%5E%29%5D%2B%5C%29%28+%3D+0%7C+const%29%3F%28%3B%7C+%5C%7B%29&path=&case=false&regexp=true

Quite some matches but most of them are generated by parser and should not be hard to fix.

By fix I mean splitting NS_IMETHOD into NS_IMETHOD_BASE (with virtual) and NS_IMETHOD (without virtual).

Depends on D139166

Depends on D139167

Depends on D139168

Depends on D139169

Depends on D139170

Depends on D139171

Depends on D139172

Depends on D139173

Depends on D139174

Depends on D139175

Depends on D139176

Depends on D139177

Depends on D139178

Hi Nika, NI'ing you as this touches XPCOM.

The intention is to split virtual keyword out of NS_IMETHOD to make our code compatible with modernize-use-override (which is basically "use virtual or override but not both").

Current:

class Foo {
  NS_IMETHOD Bar();
};

class Bar: public Foo {
  NS_IMETHOD Bar() override;
};

And my patches:

class Foo {
  virtual NS_IMETHOD Bar();
};

class Bar: public Foo {
  NS_IMETHOD Bar() override;
};

Please ping me if you have some concerns, thank you!

Depends on D139179

It feels pretty weird that we'd allow non-virtual NS_IMETHOD. Can we instead fix the lint so that it doesn't complain about keywords coming from macro expansion?

"Fix the lint" or maybe "add the lint" to force NS_IMETHOD be virtual? (I believe modifying a clang builtin lint is not very recommended and I don't think it's doing wrong)

The earlier version of my patches used #define NS_IMETHOD_BASE virtual NS_IMETHOD, that should also work. (Or maybe not...)

(In reply to Kagami :saschanaz from comment #18)

"Fix the lint" or maybe "add the lint" to force NS_IMETHOD be virtual?

I mean, that'd be another option I guess.

(I believe modifying a clang builtin lint is not very recommended and I don't think it's doing wrong)

There's tons of clang warnings that behave differently to support macro expansion IIRC, so maybe such a thing would be acceptable upstream.

Hmm, in that case I'll at least file an issue.

Okay, I now think comment #17 makes sense, actually a lot. Filed https://github.com/llvm/llvm-project/issues/53949. Feel free to add some comment if something is missing, thanks!

(wrong link right?)

Sure, sigh. Thank you.

https://hg.mozilla.org/mozilla-central/rev/cbe30b080d1b

Long time no activity there. We might want to copy-paste their rule and tweak it.

https://hg.mozilla.org/mozilla-central/rev/40a558cf1ff3

The leave-open keyword is there and there is no activity for 6 months.
:saschanaz, maybe it's time to close this bug?
For more information, please visit auto_nag documentation.

Removing NS_IMETHOD would also allow this, but per nika we can't do that before some assembly expert adjust stdcall specific parts. Bug 662348 has a patch but I don't think I can review that.